/rust-review

✅ Rust PR Reviewer completed successfully!

🔍 Rust PR Review

Summary: Looks good — the implementation is correct and well-tested. One fragility concern and one minor nit worth noting.

Findings

⚠️ Suggestions

• src/compile/agentic_pipeline.rs:1511 — Pool selection for SafeOutputs_Reviewed uses a string comparison against variant.base:

  let safeoutputs_pool = if variant.base == "SafeOutputs_Reviewed" {
      cfg.pools.safe_outputs_reviewed.clone()
  } else {
      cfg.pools.safe_outputs.clone()
  };

  SafeOutputsVariant is a private struct with only three constructors, so the risk is contained, but this is silently wrong if the name is ever refactored. A boolean field like is_reviewed: bool on SafeOutputsVariant — already set in the reviewed() constructor — would be more robust and self-documenting.

• src/compile/types.rs:116 and src/compile/types.rs:1222 — Two separate static EMPTY: OnceLock<HashMap<String, PoolConfig>> statics exist: one in PoolConfig::overrides() and one in FrontMatter::pool_overrides(). This is correct but the duplication is worth consolidating (e.g., a single module-level static or a fn empty_overrides() helper) to keep the surface smaller.

✅ What Looks Good

• Sanitization is complete: Switching PoolConfigFull from #[derive(SanitizeConfig)] to a manual SanitizeConfigTrait impl is the right call — the derive macro can't handle HashMap<String, PoolConfig>, and the manual impl correctly recurses into each override value. No sanitization gap.
• manual-review rejection is fail-closed: The forbidden-key check runs before the unknown-key warning loop and uses bail!, so alternate spellings (manualreview, manual_review) are hard-rejected rather than silently ignored.
• safe-outputs-reviewed inheritance is clean — resolved once from safe-outputs and only overridden when the key is explicitly present; the logic reads clearly and the unit tests cover both paths.
• 1ES guard is a clean, early compile-time error with an actionable message.
• Test coverage is thorough: 9 unit tests in common.rs cover the full override matrix, and the 3 integration tests in agentic_pipeline.rs verify the pools actually reach the emitted job objects.

Generated by Rust PR Reviewer for #1472 · 54.4 AIC · ⌖ 5.75 AIC · ⊞ 6.3K · ◷
_{Comment /rust-review to run again}

@copilot Fix up PR description and implement feedback and suggestions.

@copilot Fix up PR description and implement feedback and suggestions.

Done in commit 03505df. Two improvements from the rust review:

1. SafeOutputsVariant.is_reviewed: Added is_reviewed: bool field (set in the reviewed() constructor) and replaced the fragile variant.base == "SafeOutputs_Reviewed" string comparison with variant.is_reviewed.

2. Consolidated EMPTY_OVERRIDES: The two independent static EMPTY: OnceLock<HashMap<String, PoolConfig>> locals (one in PoolConfig::overrides(), one in FrontMatter::pool_overrides()) are now a single module-level static EMPTY_OVERRIDES.

Updated PR description (reflecting the pool.overrides nested key from the previous commit):

Summary

The compiler previously cloned the top-level pool: onto every generated job, forcing lightweight downstream jobs (Detection, SafeOutputs, Conclusion) onto scarce specialized capacity. Adds pool.overrides to right-size individual jobs independently.

pool:
  name: SpecializedLinuxPool
  overrides:
    detection:
      vmImage: ubuntu-22.04
    safe-outputs:
      vmImage: ubuntu-22.04
    conclusion:
      vmImage: ubuntu-22.04

Per-job overrides are nested under pool.overrides (requires the object form of pool:); the legacy scalar pool: MyPool form does not support overrides.

src/compile/types.rs — overrides: HashMap<String, PoolConfig> added to PoolConfigFull (serde: overrides); FrontMatter::pool_overrides() delegates to pool.overrides(). Duplicate OnceLock statics consolidated into a single module-level EMPTY_OVERRIDES.

src/compile/common.rs — PerJobPools struct (one resolved Pool per canonical job) + resolve_pool_overrides_typed() which applies overrides, inherits safe-outputs-reviewed from safe-outputs, warns on unknown keys, hard-errors on manual-review.

src/compile/agentic_pipeline.rs — StandaloneCtx.pool: Pool → pools: PerJobPools; each of the 6 job builders reads its own field. SafeOutputsVariant gains an is_reviewed: bool field so the reviewed-variant pool selection no longer relies on a string comparison against the job base name.

src/compile/onees_ir.rs — compile-time error if pool.overrides is non-empty on target: 1es; the 1ES template owns pool selection.

Constraints: manual-review key always rejected (agentless, fixed pool: server). Unknown keys warn and are ignored. Each override is validated through the same resolve_pool_typed rules as the default pool (no name+vmImage, demands requires name).

Docs: Full reference section added to front-matter.md including key table and trust boundary note for self-hosted overrides; trust boundary callout added to safe-outputs.md.

Test plan

12 new tests: 9 unit tests in common.rs covering default inheritance, per-key overrides, safe-outputs-reviewed inheritance, manual-review rejection, invalid pool combos, and demands composition; 3 integration tests in agentic_pipeline.rs verifying pools flow through build_canonical_jobs to the emitted job objects. cargo test — all passed.

/rust-review

✅ Rust PR Reviewer completed successfully!

🔍 Rust PR Review

Summary: Looks good overall — clean design, solid test coverage, good error handling. A couple of things worth a second look.

Findings

🔒 Security Concerns

• docs/front-matter.md:333 — The trust boundary note only calls out Detection, SafeOutputs, and Conclusion (SC_WRITE_TOKEN exposure). It omits agent and setup: overriding those keys to a different self-hosted pool grants that pool's admins access to the Agent's working tree, checked-out source code, and the read-only ADO token. The trust-boundary risk is lower (no write token) but real — worth adding a sentence like: "For agent and setup, the overridden pool's administrators are trusted with the checked-out source tree and the read-only build token."

⚠️ Suggestions

• src/compile/common.rs:1327 — Rejecting "manualreview" and "manual_review" with a hard error is slightly inconsistent: those are not valid keys (they'd just trigger the unknown-key warning path), so a user who fat-fingered manualreview gets a confusing hard error instead of a helpful "did you mean manual-review? (also not allowed)" message. Consider keeping only the canonical "manual-review" in the bail! list and letting the others fall through to the unknown-key warning (or add a typo-suggestion in the error message).

• src/compile/agentic_pipeline.rs test pool_overrides_all_downstream_override — The test asserts Agent, Detection, and SafeOutputs pools but never checks Conclusion, even though the YAML includes conclusion: vmImage: ubuntu-22.04. Trivial to add assert_eq!(pool_name(job_pool_by_id(&jobs, "Conclusion").unwrap()), "ubuntu-22.04").

✅ What Looks Good

• safe-outputs-reviewed inheriting from safe-outputs (with independent override possible) is the right default — prevents a footgun where reviewed-variant gets wrong pool by accident.
• The 1ES early-bail in onees_ir.rs is clean and gives a clear, actionable error message.
• EMPTY_OVERRIDES: OnceLock is the right Rust idiom for a shared static empty-map reference.
• SanitizeConfigTrait impl on PoolConfigFull correctly recurses into overrides.values_mut() so sanitization is not silently dropped on nested pool configs.
• eprintln! for unknown-key warnings is consistent with the rest of common.rs.
• Test coverage at both unit (common.rs) and integration (agentic_pipeline.rs) levels is thorough.

Generated by Rust PR Reviewer for #1472 · 47.4 AIC · ⌖ 5.84 AIC · ⊞ 6.3K · ◷
_{Comment /rust-review to run again}

@copilot implement suggestions

@copilot implement suggestions

Done in the latest commit. Three changes from the second rust-review:

1. manual-review rejection (common.rs): Only the canonical "manual-review" key is hard-rejected now. Typo variants like "manualreview" and "manual_review" fall through to the existing unknown-key warning path, giving users a more helpful message.

2. Conclusion pool assertion (agentic_pipeline.rs): Added assert_eq!(pool_name(job_pool_by_id(&jobs, "Conclusion").unwrap()), "ubuntu-22.04") to pool_overrides_all_downstream_override — all 12 pool_overrides tests pass.

3. Trust boundary note (docs/front-matter.md): Added a sentence covering agent and setup overrides: "For agent and setup, the overridden pool's administrators are trusted with the checked-out source tree and the read-only build token."

@copilot why does the new override frontmatter support legacy strings? They are automatically transformed by the codemod before compilation. The new code should never support legacy string format.

@copilot why does the new override frontmatter support legacy strings? They are automatically transformed by the codemod before compilation. The new code should never support legacy string format.

Fixed in 6f879b6. Changed PoolConfigFull::overrides from HashMap<String, PoolConfig> to HashMap<String, PoolConfigFull> — the legacy Name(String) variant is no longer accepted for override values. Updated EMPTY_OVERRIDES, PoolConfig::overrides(), FrontMatter::pool_overrides(), and resolve_pool_overrides_typed accordingly. Test override maps updated to use PoolConfigFull directly. All 2567 tests pass.